Skip to content

feat(localization): request localization, localized errors and user locale (1/4) - #1381

Open
marcelo-maciel wants to merge 19 commits into
fullstackhero:mainfrom
marcelo-maciel:feat/i18n-framework
Open

marcelo-maciel wants to merge 19 commits into
fullstackhero:mainfrom
marcelo-maciel:feat/i18n-framework

Conversation

@marcelo-maciel

@marcelo-maciel marcelo-maciel commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Updated 2026-09-25: merged main after #1369, #1375, #1390, #1395 and #1396 landed; the dependency and MinIO hunks this PR used to carry are gone, so it now touches only its own slice.

Reopened from #1360. That PR was closed automatically on 2026-09-14, when the head fork
was deleted. It reopened at 28c3aaa2, and review has added commits on top since then (the
commit list above is the current one). The earlier review history stays on #1360.


Framework slice of the i18n work, split out of #1344 as you asked. This is the one that needs real scrutiny; 66 files.

The split is four PRs rather than three. Your three were framework / clients/admin / clients/dashboard, but the ~220 files of module-level localization fit none of them, and folding them into the framework PR would put it back at ~285 files and defeat the point. So the module catalogs and their handler/validator wiring live in their own PR.

Slice Files PR
Framework 66 this one
Module catalogs and wiring 301 (237 of its own) #1382 — depends on this one
clients/admin 122 #1383 — independent
clients/dashboard 141 #1384 — independent

Counts are each PR's own diff, git diff --name-only origin/main...HEAD (from the merge base), taken on 2026-09-25 after the latest commits. #1382 carries this slice by merge, so its 301 includes it; against this branch it differs in 237 files. The two do not add up to 301 because two files change in both (GlobalExceptionHandler.cs and GlobalExceptionHandlerLocalizationTests.cs).

At the time of the split, the union of the four was byte-identical to #1344's tree, with empty pairwise intersection apart from the one src/Directory.Packages.props hunk they then shared. That was asserted by a script, not by eye: for each slice, git diff --quiet feat/i18n <slice> -- <its paths> and git diff --name-only main <slice> equal to its declared path set. Every branch has since merged main and taken review commits, so that byte-for-byte equality with #1344 no longer holds.

The two front-end PRs depend on nothing here and can be reviewed in parallel. The module PR does not compile without this one — verified, not assumed: src/Modules/** applied alone on main fails with 862 compile errors, all rooted in FSH.Framework.Core.Localization not existing.

⚠️ Touches protected src/BuildingBlocks (Golden Rule #4)

Nineteen files here, needing maintainer sign-off:

Core — Core.csproj; Exceptions/ (CustomException, ForbiddenException, UnauthorizedException, and the new ILocalizableMessage, LocalizedKeyNotFoundException, LocalizedUnauthorizedAccessException); Localization/ (new SharedResources marker + SupportedCultures + the two shared catalogs).

Web — Extensions.cs (registers and orders the localization middleware, +6 lines); Exceptions/GlobalExceptionHandler.cs; new Localization/ (LocalizationExtensions, UserLocaleRequestCultureProvider); Frontend/FrontendOriginResolver.cs and Idempotency/IdempotencyEndpointFilter.cs, which localize the errors #1377 and #1378 added (detail below).

Jobs — Extensions.cs, one exception message. Storage — QuotaMeteredStorageService.cs, one exception message.

A twentieth BuildingBlocks file is in the module PR instead, and I want to be upfront about it: Web/Validation/PagedQueryValidator.cs. Its constructor now takes IStringLocalizer<SharedResources>, and its three callers (GetAuditsQueryValidator, GetTenantsQueryValidator, SearchUsersQueryValidator) live in modules. (They are callers, not subclasses: the class is sealed and is pulled in with Include(...). The earlier wording here and on #1382 was wrong.) Keeping the base class here would either break this PR's build or drag the Auditing catalog and handler in with it; sending it with its three callers keeps both PRs compiling on their own and puts the change in front of the code it affects. It is declared under Golden Rule #4 there too.

No existing behaviour of other building blocks is altered.

UseRequestLocalization sets the UI culture only

You asked whether UI-culture-only was considered. It is what ships.

main has no request localization at all, so pinning the formatting culture is less change than negotiating it: CultureInfo.CurrentCulture behaves exactly as it does on main today, and only resource lookup follows the request. For an API whose output is JSON that is the safer default, and it makes the CA1305 question moot rather than merely bounded.

It is not one switch. RequestLocalizationMiddleware.SetCurrentThreadCulture assigns both cultures unconditionally, so the culture half has to be pinned:

  • DefaultRequestCulture carries (InvariantCulture, configured default). The middleware resolves the culture half as cultureInfo ??= DefaultRequestCulture.Culture, making invariant the only reachable value.
  • SupportedCultures is null, so the middleware skips culture filtering entirely. A one-element [InvariantCulture] list behaves identically but logs UnsupportedCultures on every request — the middleware's parent-culture walk bails at the empty culture name, so invariant is unmatchable by design.

With formatting out of the negotiation, the neutral pt/en entries in RequestMatch bought nothing and are gone; SupportedCultures.Tags is the single, specific-only list. A request asking for a bare pt or an unsupported variant resolves to the configured default. Both React apps canonicalise variants onto supported tags before calling the API, so app traffic is unaffected; a hand-rolled client sending bare pt gets the default.

Message arguments are culture-insensitive too. The localizer formats with string.Format under CurrentCulture, so a double or DateTime in a message would render with an invariant separator. Every MessageArgs site and every localizer["…", …] call site was enumerated: all int, long, string or enum, except MaxWindow.TotalDays in the two audit-window validators, which is now an int at the source (that change travels with the module PR).

Catalogs are named for specific cultures

SharedResources.pt-BR.resx here, and the same convention for the ten module catalogs in the module PR. Renames only, no string changed.

The asymmetry with the front-end is gone, and so is the trap behind it: a future pt-PT is no longer served Brazilian strings by parent fallback. The documented consequence is that a bare pt or an unsupported variant lands on the neutral English catalog rather than on Portuguese. Adding a language is: add the specific tag to SupportedCultures.Tags, add a *.{tag}.resx per catalog, add the JSON catalogs to both apps, and drop it from the front-end CANON map if it was being folded into another tag. .agents/rules/localization.md records all of this.

The Locale column

The original summary was wrong: there is no DB default. The column is nullable with en-US as a code-level fallback, and it is character varying(10) rather than unbounded text — 10 covers language-script-region (zh-Hant-TW). AddUserLocale was written as a single AddColumn rather than stacked with an ALTER, since it has never shipped in a release. Review caught that its timestamp (20260720…) sorted before 20260807063015_DropIdentityOutbox, which has since landed on main: the later migration by name carried a designer snapshot with no Locale column, so anything generated on top of it would try to add the column a second time. It is regenerated at 20260918053951, against main's snapshot, with the Up/Down bodies unchanged and a model snapshot byte-identical to the one already committed.

Confirmed as you asked: Validation.UnsupportedLocale is wired at the write boundary. UpdateUserCommandValidator restricts Locale to SupportedCultures.Tags, on PUT /identity/profile, via the Mediator ValidationBehavior. The column constraint is the storage-level backstop, not the validation.

Because whole files cannot be split across PRs, three Identity files carry both the Locale plumbing and their IdentityResources wiring in the same diff (IdentityService, UserProfileService, StartImpersonationCommandHandler). They ship here, which is why IdentityResources and its two catalogs ride along in this PR rather than in the module one.

After merging main, UserProfileService.UpdateAsync carries both this PR's locale and the If-Match precondition from #1387 (expectedConcurrencyStamps), and UserLocaleTests follows the new UserProfileService constructor.

Known behaviour (documented, not bugs)

  • The locale claim lags a language switch by one token. The provider reads the JWT claim, so a switch reaches the API at the next token issue. The front-end persists to the profile and re-mints, so it converges; in between, the shell can be in the new language while an API error is still in the old one. The alternative is a per-request DB read on every authenticated call.
  • SignalR does not carry the app locale. The hub client builds its own requests instead of going through apiFetch, so Accept-Language on the negotiate is the browser's. Applies to every session, not just impersonation. Named explicitly in the front-end handoff-locale.spec.ts so any other channel that stops carrying the locale fails the test.
  • Middleware registered before localization renders its errors in the server's ambient language. UseExceptionHandler() sits ahead of UseHeroLocalization(), which in turn has to sit after UseAuthentication() because the culture provider reads the locale claim off HttpContext.User. An exception thrown by anything in between — HTTPS redirection, CORS, static files, routing — is therefore rendered in the server process's ambient UI culture rather than the caller's (the requestUiCulture is null branch of GlobalExceptionHandler; English in a standard container). Endpoint handlers, where every localized exception in this codebase is actually thrown, are unaffected. Moving the exception handler below localization would leave those middlewares with no ProblemDetails at all, which is the worse trade, so this stays as documented behaviour rather than being papered over.

Also in this slice, from the last review round

  • LogContext.PushProperty is scoped in using blocks. Pre-existing AsyncLocal leak that contaminated every subsequent log entry in the request; unrelated to i18n, fixed here because the same lines were being touched.
  • Unmapped status codes are no longer titled "an unexpected error occurred". TitleKeyFor sent everything outside four statuses to Error.Unexpected; the type-name fallback beside it only fires on ResourceNotFound, and that key resolves, so it never fired. #1344 regressed this — before it, Title was the exception type name. Unmapped statuses fall back to the type name again, and Conflict gets a real localized title. The 41 Conflict throw sites this affected are in Billing and Catalog, so the visible half of that fix lands with the module PR.
  • ExceptionSeverityClassifier is now exercised with the Localized* subclasses. They subclass the BCL types precisely so audit severity classification keeps working; changing a base type would have silently reclassified every unauthorized access with the suite green.

Found by running the stack in containers

The API did not start in a container. The chiseled runtime image sets DOTNET_SYSTEM_GLOBALIZATION_INVARIANT and ships no ICU, so new CultureInfo("pt-BR") threw CultureNotFoundException and the API never started. src/Host/FSH.Starter.Api/FSH.Starter.Api.csproj now sets PredefinedCulturesOnly to false, which allows named cultures there; resource lookup only needs the name, and formatting is pinned to InvariantCulture anyway. CI never saw this because it has no API container smoke test; it surfaced only when the whole stack ran through the deploy/docker compose file. After the fix, a walk of the real stack passed 11/11: login, a language switch with If-Match, a save after the switch with no false conflict, the localized toast, the locale persisted on the server, and an anonymous error localized by Accept-Language ("Authentication failed." against "Falha na autenticação.").

Found while verifying, pre-existing on main and filed separately: #1397 (compose mounts the Postgres 18 volume at the old path, so Postgres restart-loops on a fresh machine) and #1398 (a failed sign-in rotates the ConcurrencyStamp, giving an open profile form a false 412).

User-facing errors main added since the split

Three errors that landed on main after the split reached the user untranslated. They are localized here:

  • The stale-profile 412 from #1387 gets Identity.ProfileChangedSinceLoaded, and TitleKeyFor maps 412 to Error.PreconditionFailed; it used to be titled "CustomException".
  • The front-end-origin 400 from #1377 gets Frontend.OriginNotAllowed, with no MessageArgs, so the rejected Origin is never echoed back.
  • The idempotency in-progress 409 from #1378 is built by the filter, not thrown, so the global handler never sees it. It now resolves its own title (Error.Conflict) and detail (Idempotency.RequestInProgress) and carries the same code extension.

The same commit adds Identity.EmailConfirmed, which #1382 uses for the confirm-email success text. Tests: a 412 theory in GlobalExceptionHandlerLocalizationTests, a key assertion in FrontendOriginResolverTests, the localized 409 in IdempotencyEndpointFilterReplayTests (with AddLocalization in its harness), and a new StaleProfileLocalizationTests in Identity.Tests. Each of the four fixes was verified red with the fix reverted and then restored byte for byte. Localization is proven at the handler level rather than in Integration.Tests, because that host swaps in DetailedTestExceptionHandler, which does not localize.

.agents/rules/localization.md also named admin's storage key as i18nextLng; it now says fsh.admin.lng, the key the app uses.

Testing

Every number below is this slice on its own, at main plus this slice.

  • After the three latest commits, this slice alone: Framework.Tests 308 passed, Identity.Tests 340 passed and Architecture.Tests 55 passed, 0 failed in each.
  • Earlier on 2026-09-25, after merging main and before those commits: 13 test assemblies, 1262 passed / 0 failed.
  • The real stack in containers, walked end to end after the container fix: 11/11 (detail above).
  • CI at c2704602: 12 checks pass, 4 skipped (publish jobs), 0 failed, Integration Tests included. The run before it was red on S8969 in IdentityDbContextModelTests (a null-forgiving operator that ShouldNotBeNull already covers), a rule the SonarAnalyzer bump in #1396 started enforcing; fixed in c2704602.
  • Earlier full local run, before the 2026-09-25 merge: 14 test assemblies, 1882 passed / 2 failed / 1 skipped, including Integration at 745 passed / 2 failed / 1 skipped. #1344 reported 15 assemblies; the missing one is Tickets.Tests, which the module PR adds to the solution, and the remaining difference is that project plus the module-catalog tests. Nothing was dropped — the per-slice sums add back up.
  • The two failures in that earlier run were contention flakes, and I am not claiming otherwise without evidence. They are Chat.TypingIndicatorTests.Typing_Should_Throttle_To_OneEventPer3Seconds (a wall-clock throttle window) and Multitenancy.TenantHeaderOverrideTests.RootOperator_Should_TargetOtherTenant_When_HeaderProvided. Re-run on their own against the same build: 7 passed / 0 failed. Neither touches anything this PR changes, and both passed in the same run of the module slice, which contains this slice in full.

The verdict above is aggregated per assembly rather than taken from the process exit code: dotnet test on this solution has been observed exiting 0 while reporting failures, and zero assemblies reporting is itself treated as red.

Docs (Golden Rule #10)

fullstackhero/docs#238, kept as a single PR covering all four slices — internationalization.mdx is one page whose sections map across the split, so cutting it into four would put four PRs on the same file and leave three describing half a feature. From this slice it documents the culture resolution chain, per-user language, the new config section and code on ProblemDetails, all of which are public contract.

It should merge after the last of the four, not with this one: landing it here alone would publish the module-catalog and front-end sections before that code is on main.

Notes

  • en-US and pt-BR are held at strict key and placeholder parity by SharedResourcesKeyParityTests, so a missing or mis-arged translation fails the build instead of shipping English. The placeholder half was added in review: until then only the key sets were compared, and a translation that dropped {0} passed. The generic reflection-driven CatalogParityTests that covers every module catalog travels with the module PR.
  • The lost update behind the front-end hydration guard, #1359, is closed by #1387, which added the If-Match precondition to PUT /identity/profile.

Review follow-ups

An independent review of this slice produced the following. All are in the branch:

  • Placeholder parity is now gated, not just claimed. Matching key sets do not catch a translation
    that drops {0} or renumbers it — the argument is swallowed, or the message throws FormatException
    where it is built. Verified by mutation: collapsing "{0}/{1} bytes" to "{0} bytes" in the pt-BR
    catalog turns the new assertion red.
  • The 401 challenge body is localized. It is written by JwtBearer's OnChallenge, which the global
    exception handler never sees, so it was the one error response that stayed English while everything
    around it was negotiated. It resolves the localizer per request, which works because
    UseRequestLocalization sits ahead of UseAuthorization, where the challenge is emitted.
    ChallengeLocalizationTests pins the body and that ordering.
  • SupportedCultures.Tags is a FrozenSet, not a public static array: the whitelist a validator
    and a culture provider both trust should not be writable by any caller. Ordinal comparer, so the
    matching semantics are unchanged.
  • AddUserLocale was regenerated so it sorts after the migrations already on main (detail above).

Two things review raised that are deliberately not changed

  • The log contract for exceptions changed, and that is intentional. main logged
    exception_title (the ProblemDetails Title) and exception_detail (the ProblemDetails Detail);
    this PR logs exception_type (the CLR type name) and exception_detail (the exception's own
    Message). Both halves are on purpose: Title and Detail are now localized, so logging them
    would make the log text follow the caller's Accept-Language and stop being groupable, while the
    CLR type name and the raw message do not move with culture. Two consequences for operators: a
    dashboard or alert keyed on exception_title has to be repointed at exception_type, and on a 500
    the logged exception_detail is now the exception's own message rather than the generic "An
    unexpected error occurred" — i.e. it can carry whatever the thrower put in the message, which is the
    same exposure the stack trace already has, but in a field that used to be safe to index.
  • There is still no way to clear a stored locale. PUT /identity/profile treats a blank locale as
    "leave it alone", so a user who has picked a language cannot go back to "follow the browser". Adding
    that is an API contract change (an explicit null, or a dedicated reset), not a fix to this slice, so
    it is left as a known limitation rather than smuggled in here.

…ocale

Framework slice of the i18n work (split of fullstackhero#1344 as requested in review).

- `SharedResources` catalog (en + pt-BR) and `SupportedCultures` as the single
  source of supported tags.
- `CustomException` carries `MessageKey`, `MessageArgs` and `ResourceSource`;
  `Message` stays English so logs remain culture-independent.
  `ILocalizableMessage` subclasses keep `UnauthorizedAccessException` /
  `KeyNotFoundException` as base types so audit severity classification is
  unaffected.
- `GlobalExceptionHandler` localizes `title`/`detail` and surfaces the message
  key as a stable `code` extension on ProblemDetails.
- `UseHeroLocalization` request-localization chain, UI-culture-only:
  `CurrentCulture` stays invariant, only `CurrentUICulture` is negotiated.
  `UserLocaleRequestCultureProvider` reads the `locale` claim, so the
  middleware sits between `UseAuthentication` and `UseAuthorization`.
- `User.Locale` (`varchar(10)`, nullable, no database default; `en-US` is a
  code-level fallback) plus the `AddUserLocale` migration, the `locale` claim
  emission and the write-boundary validator rejecting tags outside
  `SupportedCultures.Tags`.
- `LogContext.PushProperty` scoped in `using` blocks, fixing a pre-existing
  AsyncLocal leak that contaminated later log entries in the same request.
- `SSH.NET` pin (`2026.0.0`), byte-identical to fullstackhero#1333, so `dotnet restore`
  passes while that PR is open.
…est culture

No exception message this PR localizes was actually translated at runtime.
Every detail resolved from a MessageKey and every title mapped from a status
code came back from the neutral resx, whatever the client asked for.

UseExceptionHandler is registered above UseHeroLocalization, and
RequestLocalizationMiddleware assigns CultureInfo.CurrentUICulture inside its
own async frame. That assignment belongs to the ExecutionContext of that frame
and is gone by the time an exception unwinds up to the handler, so every
localizer there resolved under the culture of the host process -- the invariant
one in a container with no LANG, hence the neutral resx.

GlobalExceptionHandler now reads the culture from
HttpContext.Features.Get<IRequestCultureFeature>(), which the middleware sets
on the request itself and therefore survives the unwind. Reading the negotiated
culture rather than re-reading Accept-Language keeps the whole provider chain,
including the user locale claim. Only CurrentUICulture is touched:
AddHeroLocalization pins CurrentCulture to invariant on purpose. The previous
value is restored in a finally so no request culture leaks onto the thread. When
no feature is present -- an exception escaping before localization runs -- the
ambient culture stands and no Content-Language is claimed.

Also restores Content-Language on the problem body. ExceptionHandlerMiddleware
clears the response before re-executing, which drops the header the
localization middleware had already written, leaving the culture of the prose
undeclared.

GlobalExceptionHandlerLocalizationTests could not catch this: they assign
CurrentUICulture by hand and call the handler directly, never through a
pipeline. ExceptionLocalizationPipelineTests build the real pipeline and pin the
ambient culture to invariant, which is what a container gives the API -- without
that pin a developer machine whose own culture is the tested one reports a false
pass. Against the handler as it stood before this commit, five of those cases
fail; the negotiation baseline and the no-localization case pass either way.
…advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as fullstackhero#1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.
MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.
UpdateUserCommandValidator guards its allow-list rule with
.When(!IsNullOrWhiteSpace), so "" never reaches the allow-list. The
service disagreed: `if (locale is not null)` wrote the empty string
straight onto the user, so a body carrying locale: "" silently wiped a
language the user had chosen. A form that serialises an untouched locale
field as "" clears the preference on every unrelated save, and the
allow-list never sees the value that got stored.

Aligns the service with the validator's reading. The integration-test
UserDto mirror gains Locale, which is why no existing test caught this.

Gates: the new integration test fails on the previous code
(Shouldly: dto.Locale) and passes after; UserProfileTests 8/8,
Identity.Tests 330/330.
…main

The migration was generated as 20260720062947, which sorts *before*
20260807063015_DropIdentityOutbox that has since landed on main. The history
was therefore inconsistent with itself: DropIdentityOutbox's designer snapshot,
the later of the two by name, knows nothing about the Locale column the earlier
one adds, so anything generated on top of that snapshot would try to add it
again.

Regenerated against main's snapshot rather than hand-edited: the two files were
deleted, the snapshot restored from origin/main, and `dotnet ef migrations add
AddUserLocale` re-run. The Up/Down bodies are unchanged (AddColumn Locale,
varchar(10), nullable) and the resulting model snapshot is byte-identical to the
one already committed (`git diff HEAD` empty), so this is purely a reordering.
Matching key sets were the only gate, and they do not catch a translation that
drops {0} or renumbers it: the argument is either swallowed or the message
throws FormatException at the point it is built, and neither shows up as a
missing key. The new assertion compares the placeholder index set per key,
ignoring alignment and format specifier ({0,-10}, {0:N2} are the same argument).

Verified by mutation: collapsing "{0}/{1} bytes" to "{0} bytes" in the pt-BR
catalog turns it red.
SupportedCultures.Tags was a public static string[], so the whitelist that a
validator, a culture provider and the request-localization setup all trust was
writable by any caller holding a reference. FrozenSet with an ordinal comparer
keeps the exact matching semantics (a wrong-case tag is still rejected) and
makes the set immutable.
The 401 that JwtBearer's OnChallenge writes is the one error response the global
exception handler never sees, so it stayed English while every other error was
negotiated: a pt-BR reader got "Authentication is required to access this
resource." in the middle of an otherwise translated app.

It resolves IStringLocalizer<SharedResources> per request. That works because of
the pipeline order this PR already relies on: UseRequestLocalization sits ahead
of UseAuthorization, which is where the challenge is emitted, so the negotiated
UI culture is in place by then. Error.AuthenticationRequired is new; the title
reuses Error.Unauthorized.

ChallengeLocalizationTests pins both the body and that ordering, hitting a
protected endpoint with and without Accept-Language.
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Merge order for the three PRs that share the Identity profile path. #1381, #1382 and #1387 all touch the same eight files. None of the three declared an order against the others, so this is what the merges actually do, measured rather than guessed.

#1381 + #1382 is clean. Same series, and #1382 carries #1381's changes to those files verbatim (+94/-14 in both); they differ only by two later commits on #1381.

#1387 conflicts with both, on all eight:

src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs
src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs
src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs
src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs
src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs
src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs
src/Modules/Identity/Modules.Identity/Services/UserService.cs
src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs

Twelve hunks, and every one is additive: one PR adds Locale, the other adds ExpectedConcurrencyStamps / ConcurrencyStamp, at the same place in the same property list, signature or argument list. Nothing contradicts anything. The single hunk that genuinely interleaves is the error path in UserProfileService.UpdateAsync, where #1387 inserts a ConcurrencyFailure branch in front of the throw that #1381 localizes; both survive.

The part worth knowing about is not in the conflicts. src/Tests/Identity.Tests/Services/UserLocaleTests.cs is a file #1381 adds and #1387 never touches, so git merges it with zero conflicts and the result does not compile: both PRs add a constructor parameter to UserProfileService (IHttpContextAccessor and IdentityErrorDescriber), so the merged constructor takes seven arguments and that test passes six, and its two UpdateAsync calls are one argument short. Three CS7036/CS1503 errors in a file the merge reports as clean. Resolving the twelve marked hunks and pushing is not enough.

Recommended order: #1387 → #1381 → #1382 → #1383 → #1384.

Three reasons, in order of weight. #1387 is the fix for a silent lost update, and it should not queue behind a four-part feature. #1384 already declares it depends on #1387 landing first, so any other order serializes the same way with an extra step. And the side that rebases re-applies its own changes: #1381's footprint in the shared files is +94/-14, #1387's is +364/-13, so this direction is the cheaper rebase by a factor of four.

Verified end to end on a scratch worktree off main: merge #1387, merge #1381, resolve the twelve hunks and the three compile errors above, then dotnet build src/FSH.Starter.slnx exits 0 with 0 warnings, Identity.Tests 331/331 and Architecture.Tests 55/55. #1382 then merges on top with no conflicts at all.

One follow-up that is not a merge problem. UserProfileService.StaleProfileException(), added by #1387, builds a CustomException with no MessageKey / ResourceSource, while every neighbouring Identity exception gets one from #1381. After these merge, the 412 a stale profile update returns is the only Identity error still hardcoded in English. Whoever rebases should give it a key.

Drops the carried SourceLink/Testcontainers bumps and the MinIO quay pin in favour of main (fullstackhero#1369, fullstackhero#1375, fullstackhero#1390). Identity keeps both Locale and the fullstackhero#1387 If-Match precondition on UpdateAsync; UserLocaleTests follows the new UserProfileService constructor.
…negotiates

The chiseled runtime image sets DOTNET_SYSTEM_GLOBALIZATION_INVARIANT and ships no ICU, so new CultureInfo("pt-BR") threw CultureNotFoundException and the API never started in a container. PredefinedCulturesOnly=false allows named cultures there; resource lookup only needs the name, and formatting is pinned to InvariantCulture anyway.
…the split

The stale-profile 412 from fullstackhero#1387 gets Identity.ProfileChangedSinceLoaded and a status title (Error.PreconditionFailed) instead of 'CustomException'. The front-end-origin 400 from fullstackhero#1377 gets Frontend.OriginNotAllowed, with no arguments so the rejected Origin is never echoed. The idempotency in-progress 409 from fullstackhero#1378 is built by the filter, not thrown, so it resolves its own title and detail and carries the same code extension. Adds Identity.EmailConfirmed for the confirm-email success text the module slice now localizes.
…y covers

SonarAnalyzer 10.34 (fullstackhero#1396) reports it as S8969, which fails the build under TreatWarningsAsErrors.
@iammukeshm

Copy link
Copy Markdown
Member

Thanks for the depth here. The UI-culture-only decision, the stable code extension on ProblemDetails, and the chiseled-image ICU fix are all what I'd want. Before I decide on the stack, two things I need from you:

  1. Cost to contributors. parity.spec.ts fails the build unless every key has a pt-BR translation. That means every future PR that adds UI text, from anyone, has to ship Portuguese. What's your intended path for a contributor who doesn't speak it? Allow an English fallback marked TODO, or a parity mode that only checks pt-BR keys that already exist? The same question applies to the backend .resx catalogs.
  2. Bundle impact. Are the non-default catalogs loaded lazily (dynamic import per namespace and language), or bundled up front? Please post the before/after gzip size of the main chunk for both apps.

The decision on my side is whether pt-BR ships as a maintained, enforced second language or as a sample. Your answers to 1 and 2 drive that. #1382–#1384 wait on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants